Skip to content

Adding different lx layout to micro-benchmarks - #807

Open
bringlein wants to merge 2 commits into
torch-spyre:mainfrom
bringlein:ngl_fix_microbenchmarks
Open

Adding different lx layout to micro-benchmarks#807
bringlein wants to merge 2 commits into
torch-spyre:mainfrom
bringlein:ngl_fix_microbenchmarks

Conversation

@bringlein

@bringlein bringlein commented Sep 8, 2026

Copy link
Copy Markdown
Collaborator

Description

To be able to benchmark #783

Related Issues

Test Plan

Checklist

  • I have read the contributing guidelines
  • My code follows the project's code style (run bash format.sh)
  • I have added tests for my changes (if applicable)
  • I have updated the documentation (if applicable)
  • My commits include a Signed-off-by: line (DCO compliance)

Signed-off-by: Burkhard Ringlein <ngl@zurich.ibm.com>
Co-authored-by: Opus 5 / Kilo
@bringlein
bringlein requested a review from a team as a code owner September 8, 2026 17:44
@github-actions

github-actions Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

👋 Hi! Thank you for contributing.
Just a reminder: Make sure that your code passes all the linting checks, otherwise your PR won't be able to be merged. To do so, run ./format.sh.
Now you are good to go 🚀.

We also recommend installing prek and configuring it to check your code before every local commit.

@joerunde

joerunde commented Sep 8, 2026

Copy link
Copy Markdown
Collaborator

/claude-review

(testing this out for the first time)

from spyre_inference.custom_ops.utils import convert
from spyre_inference.v1.attention.backends.spyre_attn import slot_major_kv_layout
from spyre_inference.v1.attention.backends.spyre_attn import (
head_major_kv_layout,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

head_major_kv_layout does not exist in spyre_attn.py on this branch (nor anywhere in the repo) — it is introduced by the unmerged #783. This import is unconditional at the top of build_inputs_from_requests, which runs for every config, so it raises ImportError on any invocation, including the pre-existing plain and slot_major baselines that used to work. The whole microbench is broken standalone.

The same dependency applies to _fill_folded_cache: on this branch do_kv_cache_update calls self._reshape_fn(...) with a single tensor, so passing the per-kv-head list from _folded_slot_mapping would also fail without #783.

Either land #783 first (and note the ordering), or guard the import so baseline runs still work.

Comment thread scripts/microbench/README.md Outdated
Comment on lines +91 to +93
Shipped sweep configs for the LX-vs-baseline comparison (each run twice, once with
`--kv-layout lx`): `lx_study_decode_ctx`, `lx_study_prefill`, `lx_study_decode_batch`,
`lx_study_num_blocks`. Analysis lives in `spyre_attn_analysis_lx_kv_layout.ipynb`.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These four configs and the notebook don't exist in the PR. The only config added is lx_study_chunked_prefill.json, which isn't mentioned here. So anyone following the README hits a missing-file error, and the config that ships is undocumented.

Either add lx_study_decode_ctx/lx_study_prefill/lx_study_decode_batch/lx_study_num_blocks and spyre_attn_analysis_lx_kv_layout.ipynb, or update this section to describe lx_study_chunked_prefill.json.

…anch

Folds in the pieces from the parallel lx-kv-microbench branch and lets this
land on main ahead of torch-spyre#783.

- Make the head_major_kv_layout import lazy. It was unconditional, so every
  microbench run failed to import on a checkout without torch-spyre#783, not just
  --kv-layout lx. Keeping it lazy is also what allows a pre-torch-spyre#783 baseline arm.
- Add --staging-rows / staging_rows: shrink the impl's staging buffers without
  changing the measured shape, to split kernel cost from staging cost.
- Ship the four remaining sweep configs (context, batch, mixed, pin probe).
- README: correct the kernel name to _lx_page_attn_kernel, drop references to
  configs and a notebook that are not in the tree, and list what is.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Thomas Parnell <tpa@zurich.ibm.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants